-
-
Notifications
You must be signed in to change notification settings - Fork 243
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: expose normalizeSynonyms #721
Conversation
Hi @aeneasr, |
This method is useful to get aliases for migrations working! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMHO, exporting many functions as the public function could make the internal code change harder. If this (keep it private) causes a solid problem, exporting the function could be a good thing but I can't imagine a solid issue here. Could you please add some more detailed issue descriptions?
195240e
to
f1fb87b
Compare
@sio4 any update? |
In fact, I wanted to keep functions private if there is no solid reason from the perspective of the package pop itself since that alias logic is originally for internal purposes, and it will not be easy to change the API once it was exposed. At the same time, I think it could be fine because the function to be exposed by this PR is simple and I think there is not much chance to change its behavior. One thing I want to fix is the name of the function. Actually, I am not a native speaker so my impression of the function name could be wrong but I don't think the following statement is smooth. (Since it will be exposed as fixed name, I think we need to consider it more carefully.) switch NormalizeDialectSynonyms(driverName) {
...
}
cd.Dialect = NormalizeDialectSynonyms(dialect) The function name is a verb but it does nothing, just returns a value. How about something like the below? switch DialectForSynonym(driverName) {
...
}
cd.Dialect = NormalizedDialect(dialect) I feel |
f1fb87b
to
59e3ada
Compare
|
Oh, indeed! But would be better to be Will you update the PR? If your sun is already down, I can do that for you. |
Just pushed a rename commit to speed up (you already waited too long, sorry for that! :-) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Thanks for your patience @aeneasr! I merged the PR into the development branch after renaming the function. |
No description provided.